Fix: correct ssl_st member offsets #184

blaisewang · 2022-08-25T11:45:22Z

PR to fix TLSv1.3-related issues.

I use ecapture to hook libssl used by an NGINX server, not the client side libssl.
Then I found that neither the written key log file nor PcapNG with DSB could decrypt the TLS 1.3 traffic.

Environment:

NGINX with OpenSSL 1.1.1n
Cipher suites: TLS_AES_256_GCM_SHA384
curl compiled with USE_CURL_SSLKEYLOGFILE enabled

Issues I found and fixed:

write SERVER_HANDSHAKE_TRAFFIC_SECRET instead of a second CLIENT_HANDSHAKE_TRAFFIC_SECRET to the key log file
uses Hash function found in cipher_id of the master key event in expandLabel function

After fixed the above issues, still couldn't decrypt the TLS 1.3 traffic. In key log file, client_randoms are the same for both key log file. But four values are all different.

Possible issues I found:

In OpenSSL 1.1.1d, a new pointer peer_ciphers was introduced in the ssl_st struct which may affect all offsets related to hashes and secrets?

I tried:

X-Macros from this article was used to recalculate all offsets. But the program gave different offset for client_random in the ssl3_state_st struct. The current client_random offset should be correct, as it can be verified in the Client Hello message captured by tcpdump.

Result from the X-Macros program:

ssl_offsets openssl_offset_269488367 = { 
  .ssl_session_st_master_key = 0x50,
  .ssl_session_st_master_key_length = 0x8,
  .ssl3_state_st_client_random = 0xb8,
  .ssl_st_early_secret = 0x13c,
  .ssl_st_handshake_secret = 0x17c,
  .ssl_st_master_secret = 0x1bc,
  .ssl_st_resumption_master_secret = 0x1fc,
  .ssl_st_client_finished_secret = 0x23c,
  .ssl_st_server_finished_secret = 0x27c,
  .ssl_st_server_finished_hash = 0x2bc,
  .ssl_st_handshake_traffic_hash = 0x2fc,
  .ssl_st_client_app_traffic_secret = 0x33c,
  .ssl_st_server_app_traffic_secret = 0x37c,
  .ssl_st_exporter_master_secret = 0x3bc,
}

I have no clue for now to make this function work as expected. Maybe it's strict to the environment or the designated cipher suite?

@cfc4n any suggestion?

cfc4n · 2022-08-26T05:24:34Z

offset is correct. I think that hkdf code has a bug . I'll debug it later, maybe next mouth.

blaisewang · 2022-08-26T07:19:52Z

offset is correct. I think that hkdf code has a bug . I'll debug it later, maybe next mouth.

SSL_S3_CLIENT_RANDOM_OFFSET in masterkey_kern.h is set to 0xD8 or 40 (which should be 0xB8 in OpenSSL obviously) is not used.

I was misled by this offset.

blaisewang · 2022-08-26T11:47:15Z

offset is correct. I think that hkdf code has a bug . I'll debug it later, maybe next mouth.

Maybe handshake_secret's offset is 0x17C instead of 0x13C? So does for the rest. I got the same offsets by offsetof(SSL, member); the macro provided by clang.

After offset correction, the function is still not working.

HKDF from scapy (https://github.com/secdev/scapy/blob/master/scapy/layers/tls/crypto/hkdf.py) returned the same result as golang.org/x/crypto/hkdf.

I am starting to assume that handshake_traffic_hash from SSL struct is not ClientHello...ServerHello described in RFC 8446 7.1 (https://www.rfc-editor.org/rfc/rfc8446#section-7.1)

cfc4n · 2022-08-26T12:49:21Z

offset is correct. I think that hkdf code has a bug . I'll debug it later, maybe next mouth.

Maybe handshake_secret's offset is 0x17C instead of 0x13C? So does for the rest. I got the same offsets by offsetof(SSL, member); the macro provided by clang.

That is right. It's my falut.

After offset correction, the function is still not working.

maybe They are changed while eCapture capture them. I'm not an expert in TLS 1.3 handshake.

but I have an idea.

used eCapture capture them.
print they value in libssl.so source code, tls13_generate_secret function in https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/ssl/tls13_enc.c
compare them, try to find different. to identify the problem . is bug at eCapture hkdf or eCapture capture offset.

blaisewang · 2022-08-26T15:23:39Z

but I have a idea.

used eCapture capture them.

print they value in libssl.so source code, tls13_generate_secret function in https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/ssl/tls13_enc.c

compare them, try to find different. to identify the problem . is bug at eCapture hkdf or eCapture capture offset.

I will try to figure it out next week

blaisewang · 2022-08-29T09:32:14Z

Here's the code I used to log some ssl_st members' value in OpenSSL. Log first 12 char in hex format for handshake_secret, server_finished_hash, and handshake_traffic_hash.

---
 ssl/tls13_enc.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/ssl/tls13_enc.c b/ssl/tls13_enc.c
index b8fb07f210..e9eb1c6ae1 100644
--- a/ssl/tls13_enc.c
+++ b/ssl/tls13_enc.c
@@ -251,6 +251,22 @@ int tls13_generate_secret(SSL *s, const EVP_MD *md,
     return ret == 0;
 }

+int ops_write(char *prefix, unsigned char *str) {
+    FILE *fptr = fopen("/tmp/openssl.log", "a+");
+
+    if (fptr == NULL) {
+        return -1;
+    }
+
+    fprintf(fptr, "%s %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
+            prefix,
+            str[0], str[1], str[2], str[3], str[4], str[5],
+            str[6], str[7], str[8], str[9], str[10], str[11]);
+
+    fclose(fptr);
+    return 0;
+}
+
 /*
  * Given an input secret |insecret| of length |insecretlen| generate the
  * handshake secret. This requires the early secret to already have been
@@ -260,9 +276,11 @@ int tls13_generate_handshake_secret(SSL *s, const unsigned char *insecret,
                                 size_t insecretlen)
 {
     /* Calls SSLfatal() if required */
-    return tls13_generate_secret(s, ssl_handshake_md(s), s->early_secret,
+    int ret = tls13_generate_secret(s, ssl_handshake_md(s), s->early_secret,
                                  insecret, insecretlen,
                                  (unsigned char *)&s->handshake_secret);
+    ops_write("handshake_secret", (unsigned char *)s->handshake_secret);
+    return ret;
 }

 /*
@@ -650,12 +668,15 @@ int tls13_change_cipher_state(SSL *s, int which)
      * Save the hash of handshakes up to now for use when we calculate the
      * client application traffic secret
      */
-    if (label == server_application_traffic)
+    if (label == server_application_traffic) {
         memcpy(s->server_finished_hash, hashval, hashlen);
+        ops_write("server_finished_hash", (unsigned char *)s->server_finished_hash);
+    }

-    if (label == server_handshake_traffic)
+    if (label == server_handshake_traffic) {
         memcpy(s->handshake_traffic_hash, hashval, hashlen);
-
+        ops_write("handshake_traffic_hash", (unsigned char *)s->handshake_traffic_hash);
+    }
     if (label == client_application_traffic) {
         /*
          * We also create the resumption master secret, but this time use the
--

I also logged the corresponding values in eCapture.
The values are only matched after offset corrections (see my changes in masterkey_kern.h). Therefore, the previous offsets are not correct.

But after the offset corrections, the decrypt still didn't work.
Now, I guess maybe we cannot obtain TLS key from only the four values.

Maybe we need to start over from the early secrets as described in this article (https://www.containiq.com/post/decrypting-ssl-at-scale-with-ebpf)?

@cfc4n You may merge my PR into a temporary branch as the fix isn't done yet.

cfc4n · 2022-08-29T10:49:16Z

pkg/util/hkdf/hkdf.go

@@ -60,7 +61,16 @@ func expandLabel(secret []byte, label string, context []byte, length int) []byte
 		b.AddBytes(context)
 	})
 	out := make([]byte, length)
-	transcript := crypto.SHA256 // TODO  fixme : use cipher_id argument
+
+	var transcript crypto.Hash


why do not use cipher_id argument?

I would say that adding parameters to the method feels a bit redundant, especially doing cipher_id bitwise operation twice. The kind of crypto.Hash can be easily deduced from the size parameter. But I am not a Golang expert, I am not sure the best practice to do this. If you insist, I'll go with your way.

in ssl/ssl_local.h line 418 ,

/* used to hold info on the particular ciphers used */ struct ssl_cipher_st { uint32_t valid; const char *name; /* text name */ const char *stdname; /* RFC name */ uint32_t id; /* id, 4 bytes, first is version */ /* * changed in 1.0.0: these four used to be portions of a single value * 'algorithms' */ uint32_t algorithm_mkey; /* key exchange algorithm */

cipher_id means algorithm type. so use this method is a best way.

var transcript hash.Hash // test with different cipher_id switch cipher_id & 0x0000FFFF { case 0x1301: t.Log("TLS_AES_128_GCM_SHA256") transcript = crypto.SHA256.New() case 0x1302: t.Log("TLS_AES_256_GCM_SHA384") transcript = crypto.SHA384.New() case 0x1303: t.Log("TLS_CHACHA20_POLY1305_SHA256") transcript = crypto.SHA256.New() default: t.Log("Unknown cipher") }

I was too busy last week. I will open another PR to fix this later

user/module/probe_openssl.go

cfc4n · 2022-08-29T10:57:25Z

thanks for your work. as you said, we need a new HOOK entry to get early secrets.

blaisewang · 2022-08-29T14:34:02Z

thanks for your work. as you said, we need a new HOOK entry to get early secrets.

Adding hook is very easy. But the real problem is that is the early_secrets really needed. I tried to implement the way described in that article last week, still didn't work (maybe my implementation is wrong). This leads to the previous mentioned question

Is handshake_traffic_hash from SSL struct the ClientHello...ServerHello described in RFC 8446 7.1 ?

From the comments of OpenSSL project, It does feel like the handshake_traffic_hash is the hash of ClientHello...ServerHello.

https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/ssl/tls13_enc.c#L601

Again, I am not a OpenSSL or TLS 1.3 expert either. So, I searched all over GitHub and I do found a project that written the same way you did.

https://github.com/KJTsanaktsidis/tlskeydump/blob/main/src/probes/openssl_probe.cxx#L267

After compiling and run the project in an unstable Debian as mentioned in the project manual. Nothing was written in the key log file, so no comparison.

I have no clue for now. Maybe searching an OpenSSL or TLS 1.3 expert for help is the best approach, or be an expert?

cfc4n · 2022-08-30T13:14:10Z

Again, I am not a OpenSSL or TLS 1.3 expert either. So, I searched all over GitHub and I do found a project that written the same way you did.

me too.

I have no clue for now. Maybe searching an OpenSSL or TLS 1.3 expert for help is the best approach, or be an expert?

I think that be an expert seems feasible.

cfc4n

merged

blaisewang added 2 commits August 25, 2022 18:15

fix: unify comment fmt

22929bc

fix: key log file label

eb0b5f4

cfc4n added 🐞 bug Something isn't working fix bug fix PR labels Aug 27, 2022

cfc4n marked this pull request as draft August 28, 2022 15:39

blaisewang added 2 commits August 29, 2022 16:39

fix: ssl_st members' offsets

620a3f5

fix: use correct hash in expandLabel

1b5f100

blaisewang force-pushed the master branch from 6b73a0f to 1b5f100 Compare August 29, 2022 08:54

blaisewang changed the title ~~WIP: fix TLS 1.3 issues~~ Fix: correct ssl_st member offsets Aug 29, 2022

blaisewang marked this pull request as ready for review August 29, 2022 09:33

cfc4n reviewed Aug 29, 2022

View reviewed changes

cfc4n added the good first issue Good for newcomers label Aug 29, 2022

fix: better use of switch case pointed by cfc4n

05921c7

cfc4n approved these changes Sep 2, 2022

View reviewed changes

cfc4n merged commit 5f1b2ce into gojue:master Sep 2, 2022

Alberthchang mentioned this pull request Feb 13, 2024

gotls报错 #482

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: correct ssl_st member offsets #184

Fix: correct ssl_st member offsets #184

blaisewang commented Aug 25, 2022 •

edited

Loading

cfc4n commented Aug 26, 2022

blaisewang commented Aug 26, 2022 •

edited

Loading

blaisewang commented Aug 26, 2022

cfc4n commented Aug 26, 2022 •

edited

Loading

blaisewang commented Aug 26, 2022

blaisewang commented Aug 29, 2022

cfc4n Aug 29, 2022

blaisewang Aug 29, 2022

cfc4n Aug 30, 2022

blaisewang Sep 5, 2022

cfc4n commented Aug 29, 2022

blaisewang commented Aug 29, 2022

cfc4n commented Aug 30, 2022

cfc4n left a comment

Fix: correct ssl_st member offsets #184

Fix: correct ssl_st member offsets #184

Conversation

blaisewang commented Aug 25, 2022 • edited Loading

cfc4n commented Aug 26, 2022

blaisewang commented Aug 26, 2022 • edited Loading

blaisewang commented Aug 26, 2022

cfc4n commented Aug 26, 2022 • edited Loading

blaisewang commented Aug 26, 2022

blaisewang commented Aug 29, 2022

cfc4n Aug 29, 2022

Choose a reason for hiding this comment

blaisewang Aug 29, 2022

Choose a reason for hiding this comment

cfc4n Aug 30, 2022

Choose a reason for hiding this comment

blaisewang Sep 5, 2022

Choose a reason for hiding this comment

cfc4n commented Aug 29, 2022

blaisewang commented Aug 29, 2022

cfc4n commented Aug 30, 2022

cfc4n left a comment

Choose a reason for hiding this comment

blaisewang commented Aug 25, 2022 •

edited

Loading

blaisewang commented Aug 26, 2022 •

edited

Loading

cfc4n commented Aug 26, 2022 •

edited

Loading